-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve display of enum variants #90089
Conversation
Some changes occurred in HTML/CSS/JS. |
This comment has been minimized.
This comment has been minimized.
37d9c45
to
53d5ed8
Compare
Just a quick message to say that I think that the layout is (with your changes) even more confusing. I don't know where I should put my eyes. The variant name is so big that it's disturbing, I feel that I should stop at every single of them. The all "Variant" layout feels out of place with the rest of the documentation. Maybe instead of putting the variant name in h{3,4} you could make it bolder, this would would accentuate the difference but not be as disturbing as your changes. |
Great feedback, thanks. I'm keeping the Updated the demos. |
Can you add a demo for what it looks like when some of the fields have documentation? |
I updated the xmlparser / Token demo to include documentation on the fields: https://jacob.hoffman-andrews.com/rust/xmlparser-updated/xmlparser/enum.Token.html#variants |
One thing I'd suggest is to add a bit more space between the variant heading and "Fields" and between the fields and the variant's docs. Right now it looks a bit cramped to me. |
Happy to do so! Would you mind trying out some spacing options in your developer tools and suggesting one that looks good to you? I'm happy to keep iterating on this but I suspect we'll save some round trips this way. :-) |
I was going to suggest this, but one problem is that for tuple fields, the docs might be confusing. The fields aren't actually named |
The diff from my changes: - .sub-variant > span {
- margin-left: 20px;
- }
.sub-variant {
margin-left: 24px;
- margin-bottom: 10px;
+ margin-bottom: 15px;
} plus the removal of the |
Currently they are, so I don't see this as an issue. @jsha: I find it harder to read the documentation because it's actually not that easy to find where the field starts and where its documentation starts. |
I've updated the CSS like so:
I think this should improve visual distinctiveness between the bottom of the fields and the beginning of the doccomment. I don't want to remove the "Fields" heading right now, since I think it is important for explaining the section it comes in front of. Also I'd like to point out this is intended to be a quick fix for some significant problems with the display of enum field variants (horizontal line inverts the hierarchy; fields show up below doccomment; fields heading is bold, inverting the hierarchy). I always want to try and get the details right, of course, but I also don't want to let a few pixels of whitespace take up too much time here - there are other big display issues I'd like to move on to fixing, also. |
What do you mean by "currently they are"? The issue I was describing is that in the docs, the fields are displayed as |
Ah, this looks much better, thank you! After this change, the docs feel much less cramped to me. I only have two remaining comments about the UI (haven't looked at the impl yet):
|
In other places, the doccomment is always the last part of a section, and fields / other generated stuff is at the top. Consider the top of the page, for instance - it has struct contents, or enum contents, and then the doccomment underneath. However, I don't feel strongly about this, so I've reverted to the previous behavior of fields on the bottom. I've also updated the font sizes as you suggest, and pushed the demos. |
I'm realizing now that the Can we have a symbol or symbol or something making it easier to spot the fields? |
Can you tell me more about what you mean by "identify item vs documentation?" Are you saying that the "Fields" heading and the lines below it look like they are prose documentation written by the doc author? Or that you're scanning the docblock looking for the break between prose and Rusty items, and finding that the break doesn't catch the attention enough? |
I meant that. It's hard to know if I'm still reading documentation or if it's a field. I have to stop and look a bit around to have this answer, which is not great. |
Would moving the fields docs to right below the variant's name like they were earlier in this PR make it easier for you? I agree that the fields look a bit like they're part of the docs themselves. I'd rather put the fields above the variant's docs than add more symbols to the docs though. |
Worth a try! |
Before I go back to that, I have one more idea to try: This increases the font-weight on If this is not satisfactory I'm perfectly happy going back to fields-on-top. Let me know which you prefer! |
I think this adds more complexity and actually makes the docs more confusing. The issue is that "Fields" looks like it's part of the prose, and this makes it look even more like it's part of the prose. Also, I think we should be leaning away from adding more colors and links to the docs. |
Good point, thanks. Back to fields-on-top. Demos updated - don't forget to hard-refresh. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good improvement, thanks for doing this! I'll review the code soon; for now I have one last UI suggestion.
☔ The latest upstream changes (presumably #90422) made this pull request unmergeable. Please resolve the merge conflicts. |
Use h3 and h4 for the variant name and the "Fields" subheading. Remove the "of T" part of the "Fields" subheading. Remove border-bottom from "Fields" subheading. Move docblock below "Fields" listing.
55725d5
to
69df43b
Compare
(I'll try to review this soon.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Rust changes mostly look fine to me, but I'd like @GuillaumeGomez to review the CSS/HTML tests.
} | ||
|
||
document(w, cx, variant, Some(it), HeadingOffset::H4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, should this be H3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think (iirc) that it means the biggest heading will be h4
(please someone correct me if I'm wrong).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I think the heading immediately above (in the HTML, not the Rust source) is h3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is correct. In the demo: https://rustdoc.crud.net/jsha/enum-headings/foo/enum.Token.html#variant.Declaration
"Declaration" is h3, and then "Variant-First" is h4, as it should be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I guess I'm still confused by HeadingOffset
's numbering.
Looks good to me as well. Once we have confirmation for @camelid's last message, we can approve it. :) |
Once #90089 (comment) is addressed, |
@bors r=camelid,GuillaumeGomez rollup |
📌 Commit 69df43b has been approved by |
…d,GuillaumeGomez Improve display of enum variants Use h3 and h4 for the variant name and the "Fields" subheading. Remove the "of T" part of the "Fields" subheading. Remove border-bottom from "Fields" subheading. Move docblock below "Fields" listing. Fixes rust-lang#90061 Demo: https://jacob.hoffman-andrews.com/rust/xmlparser-updated/xmlparser/enum.Token.html#variants https://jacob.hoffman-andrews.com/rust/fix-enum-variants/std/io/enum.ErrorKind.html#variants https://jacob.hoffman-andrews.com/rust/fix-enum-variants/std/result/enum.Result.html#variants r? `@camelid`
Thanks again for making this great UI improvement! |
…d,GuillaumeGomez Improve display of enum variants Use h3 and h4 for the variant name and the "Fields" subheading. Remove the "of T" part of the "Fields" subheading. Remove border-bottom from "Fields" subheading. Move docblock below "Fields" listing. Fixes rust-lang#90061 Demo: https://jacob.hoffman-andrews.com/rust/xmlparser-updated/xmlparser/enum.Token.html#variants https://jacob.hoffman-andrews.com/rust/fix-enum-variants/std/io/enum.ErrorKind.html#variants https://jacob.hoffman-andrews.com/rust/fix-enum-variants/std/result/enum.Result.html#variants r? `@camelid`
…askrgr Rollup of 8 pull requests Successful merges: - rust-lang#88361 (Makes docs for references a little less confusing) - rust-lang#90089 (Improve display of enum variants) - rust-lang#90956 (Add a regression test for rust-lang#87573) - rust-lang#90999 (fix CTFE/Miri simd_insert/extract on array-style repr(simd) types) - rust-lang#91026 (rustdoc doctest: detect `fn main` after an unexpected semicolon) - rust-lang#91035 (Put back removed empty line) - rust-lang#91044 (Turn all 0x1b_u8 into '\x1b' or b'\x1b') - rust-lang#91054 (rustdoc: Fix some unescaped HTML tags in docs) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Use h3 and h4 for the variant name and the "Fields" subheading.
Remove the "of T" part of the "Fields" subheading.
Remove border-bottom from "Fields" subheading.
Move docblock below "Fields" listing.
Fixes #90061
Demo:
https://jacob.hoffman-andrews.com/rust/xmlparser-updated/xmlparser/enum.Token.html#variants
https://jacob.hoffman-andrews.com/rust/fix-enum-variants/std/io/enum.ErrorKind.html#variants
https://jacob.hoffman-andrews.com/rust/fix-enum-variants/std/result/enum.Result.html#variants
r? @camelid